Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Kubelet and Containerd when using cgroupfs #8123

Merged
merged 1 commit into from
Nov 5, 2021
Merged

Fix Kubelet and Containerd when using cgroupfs #8123

merged 1 commit into from
Nov 5, 2021

Conversation

pasqualet
Copy link
Contributor

@pasqualet pasqualet commented Oct 24, 2021 •
edited by floryut
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fix the usage of cgroupfs with containerd

Which issue(s) this PR fixes:

Fixes #8122

Special notes for your reviewer:

You can test the PR with the following variables:

container_manager: containerd
containerd_use_systemd_cgroup: false

Does this PR introduce a user-facing change?:

[containerd] Fix the usage of cgroupfs with containerd and introduce cgroupsfs specific variables (⚠️ `containerd_runtimes` is now `containerd_additional_runtimes` )

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 24, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 24, 2021
@pasqualet pasqualet mentioned this pull request Oct 24, 2021
Merged
@cristicalin
Copy link
Contributor

Would you mind adding a CI job for this configuration and also adding a point to the docs about why/when this would be needed? I have in mind especially the discussion on #8068

@fungusakafungus
Copy link
Contributor

We use

kubelet_runtime_cgroups: /system.slice/containerd.service
kubelet_kubelet_cgroups: /system.slice/kubelet.service

and systemCgroups: /system.slice in kubelet_config_extra_args
with that containerd configuration

container_manager: containerd
containerd_use_systemd_cgroup: false

@pasqualet
Copy link
Contributor Author

@fungusakafungus Are you also setting cgroupRoot? As far I can see from the doc, this is a required field when systemCgroups is not empty.

@fungusakafungus
Copy link
Contributor

@fungusakafungus Are you also setting cgroupRoot? As far I can see from the doc, this is a required field when systemCgroups is not empty.

ah, yes, we set it to cgroupRoot: /

@cristicalin
Copy link
Contributor

I tested this on top of the Kata PR #8068 and I managed to successfully create pods with kata 2.2.2.

@cristicalin
Copy link
Contributor

@pasqualet since you are replacing the user facing inventory variable containerd_runtimes with containerd_additional_runtimes I suggest you update our docs with the relevant description.

@pasqualet
Copy link
Contributor Author

I've updated default values and the documentation.

@fungusakafungus
Copy link
Contributor

This removes configurability for the default runc runtime, I was just going to use that...

fungusakafungus
fungusakafungus reviewed Oct 26, 2021
View reviewed changes
roles/kubernetes/node/tasks/facts.yml
when: kubelet_cgroup_driver is undefined

- name: set kubelet_cgroups options when cgroupfs is used
set_fact:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could set defaults depending on kubelet_cgroup_driver|default(kubelet_cgroup_driver_detected) in defaults.yml like

# Set runtime cgroups
kubelet_runtime_cgroups: >-
  {%- if kubelet_cgroup_driver|default(kubelet_cgroup_driver_detected) == "systemd" -%}
  /systemd/system.slice
  {%- else -%}
  /system.slice/containerd.service
  {%- endif -%}

roles/kubernetes/node/defaults/main.yml Outdated
kubelet_kubelet_cgroups: "/systemd/system.slice"

# Set runtime and kubelet cgroups when using cgroupfs as cgroup driver
kubelet_runtime_cgroups_cgroupfs: "/systemd/containerd.service"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about the /systemd/ prefix? For us /system.slice/containerd.service works better(we have containerd metrics via cAdvisor from kubelet), I also just found that value here: https://github.com/kubernetes/kubernetes/blob/master/cluster/gce/util.sh#L827

Copy link
Contributor Author

@pasqualet pasqualet Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it was a mistake. Fixed.

@pasqualet pasqualet force-pushed the fix-kubelet-cgroup branch 2 times, most recently from 5130ee1 to 4ad344e Compare October 27, 2021 08:44
@pasqualet
Copy link
Contributor Author

This removes configurability for the default runc runtime, I was just going to use that...

I have created the variable named containerd_runc_runtime to cusotmize the runc runtime.

@cristicalin
Copy link
Contributor

Nice work @pasqualet! Thanks for fixing this!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2021
@floryut floryut added kind/container-managers Containers section in the release note and removed kind/bug Categorizes issue or PR as related to a bug. labels Nov 5, 2021
floryut
floryut approved these changes Nov 5, 2021
View reviewed changes
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, reminder for release note and people dropping here
containerd_runtimes no longer exists and containerd_additional_runtimes should now be used.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, pasqualet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2021
@floryut
Copy link
Member

floryut commented Nov 5, 2021

This also introduce 3 new variables when using cgroupfs

kubelet_runtime_cgroups_cgroupfs
kubelet_kubelet_cgroups_cgroupfs
kubelet_config_extra_args_cgroupfs

Will try to remember that for the release note.

@k8s-ci-robot k8s-ci-robot merged commit 6e5b9e0 into kubernetes-sigs:master Nov 5, 2021
@cristicalin cristicalin mentioned this pull request Nov 18, 2021
Closed
@pasqualet pasqualet mentioned this pull request Nov 18, 2021
Merged
@zxdvd zxdvd mentioned this pull request Dec 12, 2021
Closed
@floryut floryut mentioned this pull request Dec 21, 2021
Closed
@fungusakafungus fungusakafungus mentioned this pull request Feb 1, 2022
Merged
sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022
@pasqualet @sakuraiyuta
Fix Kubelet and Containerd when using cgroupfs as cgroup driver (kube…
8b94c58 
…rnetes-sigs#8123)
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 28, 2023
@pasqualet @LuckySB
Fix Kubelet and Containerd when using cgroupfs as cgroup driver (kube…
b667649 
…rnetes-sigs#8123)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fungusakafungus fungusakafungus fungusakafungus left review comments

@floryut floryut floryut approved these changes

@bozzo bozzo Awaiting requested review from bozzo

@holmsten holmsten Awaiting requested review from holmsten

Assignees

@cristicalin cristicalin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/container-managers Containers section in the release note lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Containerd fails to create containers when containerd_use_systemd_cgroup=false
5 participants
@pasqualet @cristicalin @fungusakafungus @k8s-ci-robot @floryut